Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for IPL-4 by implementing dynamic Astra schema selection based on data release versions. The key changes include updating schema mapping logic to handle the new IPL-4 version (0.8.0) while maintaining backward compatibility with DR19/IPL-3 (0.5.0, 0.6.0).
- Implemented dynamic Astra schema selection in database connection logic
- Enhanced query functions with proper documentation and simplified logic
- Updated data model to make certain fields optional for compatibility
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/valis/db/db.py | Added dynamic Astra schema selection based on release version in database connection dependency |
| python/valis/db/queries.py | Simplified Apogee target retrieval, added comprehensive docstrings, and enhanced schema validation |
| python/valis/db/models.py | Made several AstraSource fields optional to accommodate schema variations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
python/valis/db/queries.py
Outdated
| if not vastra or vastra not in ("0.5.0", "0.6.0"): | ||
| print('astra only supports DR19 / IPL3 = version 0.5.0, 0.6.0') | ||
| vastra = "0.5.0" if vastra in ("0.5.0", "0.6.0") else vastra | ||
| if vastra.replace('.', '') not in astra.Source._meta.schema: |
There was a problem hiding this comment.
The condition vastra.replace('.', '') not in astra.Source._meta.schema will fail when vastra is None (returned by get_software_tag when release is invalid). This will cause an AttributeError. Add a check for None before calling .replace().
| if vastra.replace('.', '') not in astra.Source._meta.schema: | |
| if vastra is None or vastra.replace('.', '') not in astra.Source._meta.schema: |
| vastra = "0.5.0" if vastra in ("0.5.0", "0.6.0") else vastra | ||
| schema = f"astra_{vastra.replace('.', '')}" |
There was a problem hiding this comment.
[nitpick] The version mapping logic vastra = \"0.5.0\" if vastra in (\"0.5.0\", \"0.6.0\") else vastra is duplicated in both get_pw_db and get_astra_target. Consider extracting this logic into a shared utility function to improve maintainability.
This PR adds support for IPL-4 into valis.